Added a helper function for bc related warp functions #98
Conversation
|
Please add the helpers to the init and import them from xlb.helper similar to the other helper functions. |
It is not possible because the DefaultConfig is not set before xlb.init is called! |
|
Then this is not ideal. The import of helper functions should not depend on calling xlb.init(). Please find a better solution or consider just consolidating these functions in the BCs. |
|
Not sure why this is an issue here. This helper function does not call xlb.init() and it cannot be used anywhere ouside of BC implementations where xlb.init() has already been called. Having everything inside init is also not a requirement for packaging. Can you give a scenario where the current implementation might cause an issue? |
|
A "helper" function should not rely on initialization and must be usable independently. Since these functions may be invoked by users in examples, please relocate them to the boundary condition base class, as their functionality aligns more closely with that context. |
|
Moving them to the base class is exteremely messy because they have to be added one by one inside class Init method and given to self. The Warp functions need to be enclosed within a python function to declare the needed constants and variables that are out of the scope of that wp.func! I think the current approach is the cleanest. Please recommend a better approach and I will gladly change it. |
|
There are few methods right now inside the bc base class that are defined inside the init. I should perhaps move them to this helper function too and import them whenever needed. That would be for this PR. If you agree I can do that as well. |
hsalehipour
force-pushed
the
bc_warp_function_helper
branch
from
f2db380 to
00b678b
Compare
|
I moved the new file from helper folder to BC folder which I think addresses the comment about "helper" function. |
hsalehipour
force-pushed
the
bc_warp_function_helper
branch
from
00b678b to
247e6ec
Compare
|
I agree that BC config should not be strictly identical to DefaultConfig. Based on the previous review I have pushed some changes which I think resolves this issue altogether. Please re-review. |
There was a problem hiding this comment.
I’m not a fan of this PR. While I’ve added some comments, this kind of change might seem like an improvement at first glance, but the added abstractions and convolutions unnecessarily complicate what were initially straightforward function calls for someone who is not familiar with this code. Since the number of lines added and removed are nearly identical, it doesn't truly abstract or simplify anything meaningful.
You can just simply revert them back to boundary_condition just as before, and move the function calls related to the BCs to their respective class. For example all functions like bounceback_noneequilibrium, regularize_fpop, _get_fsum are not really "common" functions to be placed in a separate class as they are only applicable to certain BC types. I would rather find them in their respective classes than going to multiple
I agree get_normal_vectors could be a useful function and can be placed on line 118 of boundary_condition.py.
hsalehipour
force-pushed
the
bc_warp_function_helper
branch
2 times, most recently
from
b300a54 to
6059197
Compare
|
I addressed all the concerns and pushed some new changes. Although it may seem these changes are cosmetic right now but with new BCs that I have added (under test now) these functions will be used more and it makes more sense (to me) to have them in a central place to help with debugging. |
hsalehipour
force-pushed
the
bc_warp_function_helper
branch
from
6059197 to
74c5cdf
Compare
|
addressed the latest comment and pushed again. |
hsalehipour
force-pushed
the
bc_warp_function_helper
branch
from
74c5cdf to
fd3c394
Compare
| @@ -16,6 +16,7 @@ | |||
| from xlb.operator.operator import Operator | |||
| from xlb import DefaultConfig | |||
| from xlb.operator.boundary_condition.boundary_condition_registry import boundary_condition_registry | |||
| from xlb.operator.boundary_condition.helper_functions_bc import HelperFunctionsBC | |||
There was a problem hiding this comment.
Done. Pushed it
…ical warp functions.
hsalehipour
force-pushed
the
bc_warp_function_helper
branch
from
fd3c394 to
e32a41d
Compare
Added a helper function for BCs to avoid repeated definition of identical warp functions.
Contributing Guidelines
Description
We cannot inherit methods in Warp. As a workaround, rather than repeating snippets to represent identical Warp functionalities (which is what is done currently in the code), this PR creates a helper function that keeps those methods. These methods are imported as needed inside construct_warp.
Type of change
How Has This Been Tested?
Linting and Code Formatting
Make sure the code follows the project's linting and formatting standards. This project uses Ruff for linting.
To run Ruff, execute the following command from the root of the repository:
ruff check .